Implement compute manager resource #920

ksamoray · 2023-07-12T17:00:17Z

No description provided.

vmwclabot · 2023-07-12T17:00:20Z

@ksamoray, you must sign every commit in this pull request acknowledging our Developer Certificate of Origin before your changes are merged. This can be done by adding Signed-off-by: John Doe <[email protected]> to the last line of each Git commit message. The e-mail address used to sign must match the e-mail address of the Git author. Click here to view the Developer Certificate of Origin agreement.

Commit 340d107 must be signed by [email protected]

ksamoray · 2023-07-13T18:07:51Z

/test-all

ksamoray · 2023-07-14T04:39:06Z

/test-all

ksamoray · 2023-07-17T06:20:51Z

/test-all

annakhm · 2023-07-17T19:06:41Z

nsxt/resource_nsxt_compute_manager.go

+							},
+							Elem: &schema.Resource{
+								Schema: map[string]*schema.Schema{
+									"thumbprint": {


Lets add Sensitive: true to all secrets

annakhm · 2023-07-17T19:08:11Z

nsxt/resource_nsxt_compute_manager.go

+					},
+				},
+			},
+			"origin_type": {


is this an enumeration?

No - and indeed it's quite awkward

I wonder what else can it be. Perhaps we should default to vcenter as UI does

annakhm · 2023-07-17T19:09:14Z

nsxt/resource_nsxt_compute_manager.go

+				Description: "Compute manager type like vCenter",
+				Required:    true,
+			},
+			"reverse_proxy_https_port": {


should we add ValidateFunc: validateSinglePort() for this port?

annakhm · 2023-07-17T19:09:36Z

nsxt/resource_nsxt_compute_manager.go

+				Description: "IP address or hostname of compute manager",
+				Required:    true,
+			},
+			"set_as_oidc_provider": {


Default value here?

I've added defaults to all the attributes which have default values in NSX. But I'm not sure I understand this: if TF assigns a default value, wouldn't it clutter the state file with stuff that wasn't assigned by the user?

If NSX assigns default, it will make its way to the state anyway.

annakhm · 2023-07-17T19:12:55Z

nsxt/resource_nsxt_compute_manager_test.go

+        tag   = "tag1"
+    }
+
+	server = "%s"


lets format this

ksamoray · 2023-07-18T11:10:36Z

/test-all

annakhm · 2023-07-24T18:12:46Z

nsxt/resource_nsxt_compute_manager.go

+				MaxItems:    1,
+				Elem: &schema.Resource{
+					Schema: map[string]*schema.Schema{
+						"saml_login_credential": {


I would suggest to remove credential suffix here since its already present in parent name

annakhm · 2023-07-24T18:13:52Z

nsxt/resource_nsxt_compute_manager.go

+							Description: "A login credential specifying saml token",
+							Optional:    true,
+							MaxItems:    1,
+							ExactlyOneOf: []string{


shouldn't this be defined under credential rather than under saml_login_credential?

I've looked at examples such as this, it seemed correct as it is. It behaves as expected.

annakhm · 2023-07-24T18:14:29Z

nsxt/resource_nsxt_compute_manager.go

+								Schema: map[string]*schema.Schema{
+									"password": {
+										Type:        schema.TypeString,
+										Description: "The authentication password for login",


I think username and password are required

For some reason they aren't listed as required.
Yet the UI forces to specify these.

Can we check this against NSX to see if empty creds are rejected? Same regarding other types of creds.

I think that it's a waste of time - there's no way anyone could login without creds. I made these required already.

annakhm · 2023-07-24T18:14:52Z

nsxt/resource_nsxt_compute_manager.go

+										Type:        schema.TypeString,
+										Description: "Thumbprint of the server",
+										Optional:    true,
+									},


feels like this should be required

annakhm · 2023-07-25T19:24:12Z

website/docs/r/compute_manager.html.markdown

+  server = "192.168.244.144"
+
+  credential {
+    username_password_login_credential {


credential no longer needed here

annakhm · 2023-07-25T19:24:56Z

website/docs/r/compute_manager.html.markdown

+  * `multi_nsx` - (Optional) Specifies whether multi nsx feature is enabled for compute manager.
+  * `origin_type` - Compute manager type like vCenter.
+  * `reverse_proxy_https_port` - (Optional) Proxy https port of compute manager.
+  * `server` - IP address or hostname of compute manager.


Should be marked as (Required)

annakhm · 2023-07-25T19:25:28Z

website/docs/r/compute_manager.html.markdown

+* `tag` - (Optional) A list of scope + tag pairs to associate with this resource.
+* `access_level_for_oidc` - (Optional) Specifies access level to NSX from the compute manager.
+* `create_service_account` - (Optional) Specifies whether service account is created or not on compute manager.
+* `credential` - Login credentials for the compute manager. Should contain exactly one credential enlisted below: 


This is (Required) I think?

General question of convention: (Optional) is optional, and (Required) is required, what about those who do not have any specification of neither?

It think all attributes are one of Required, Optional or RO (Computed only), in which case its moves to Attributes section

annakhm · 2023-07-25T19:27:15Z

website/docs/r/compute_manager.html.markdown

+    * `pem_encoded` - PEM encoded certificate data.
+    * `private_key` - Private key of certificate.
+  * `multi_nsx` - (Optional) Specifies whether multi nsx feature is enabled for compute manager.
+  * `origin_type` - Compute manager type like vCenter.


This is Optional now, lets specify default as well.

annakhm · 2023-07-25T19:27:49Z

website/docs/r/compute_manager.html.markdown

+* `display_name` - (Required) Display name of the resource.
+* `description` - (Optional) Description of the resource.
+* `tag` - (Optional) A list of scope + tag pairs to associate with this resource.
+* `access_level_for_oidc` - (Optional) Specifies access level to NSX from the compute manager.


Lets add the enum values and default here

annakhm · 2023-07-27T17:02:38Z

nsxt/resource_nsxt_compute_manager.go

+									"thumbprint": {
+										Type:        schema.TypeString,
+										Description: "Thumbprint of the login server",
+										Required:    true,


thumbprint is not required in UI

Indeed - instead the UI fetches the thumbprint and asks for verification.
with curl the transaction fails when the thumbprint is missing with the following error:

"Compute manager server x.x.x.x thumbprint input empty is not valid. Current retrieved compute manager server thumbprint is xx:xx:xx:xx:xx:xx:xx:xx:xx:xx:xx:xx:xx:xx:xx:xx:xx:xx:xx:xx:xx:xx:xx:xx:xx:xx:xx:xx:xx:xx:xx:xx . If correct, please re-submit with this thumbprint"

Signed-off-by: Kobi Samoray <[email protected]>

ksamoray · 2023-07-30T11:52:15Z

/test-all

annakhm · 2023-08-01T19:06:48Z

nsxt/resource_nsxt_compute_manager_test.go

+
+  credential {
+    username_password_login {
+      username = "%s"


would the API fail if credentials are incorrect? If no, I suggest to use dummy credentials here and avoid basing this test on testAccTestVCCredentials

NSX validates the credentials and tests the connection. And it should be a vCenter host which isn't already connected to NSX (maybe this NSX instance?) so IDK how we run this in CI. Unless we keep an extra vCenter node for that purpose.

annakhm

Just one comment about test precheck that can be addressed later

vmwclabot added the dco-required DCO Required label Jul 12, 2023

ksamoray force-pushed the compute_managers branch from 340d107 to f1dc682 Compare July 12, 2023 17:00

vmwclabot removed the dco-required DCO Required label Jul 12, 2023

ksamoray force-pushed the compute_managers branch 6 times, most recently from 08cb840 to 41c9bd5 Compare July 13, 2023 17:35

ksamoray force-pushed the compute_managers branch from 41c9bd5 to a998e85 Compare July 13, 2023 18:52

ksamoray force-pushed the compute_managers branch from a998e85 to 3a3beda Compare July 17, 2023 06:19

annakhm reviewed Jul 17, 2023

View reviewed changes

ksamoray force-pushed the compute_managers branch from 3a3beda to 486c0c2 Compare July 18, 2023 11:09

annakhm reviewed Jul 24, 2023

View reviewed changes

ksamoray force-pushed the compute_managers branch 3 times, most recently from 348341d to 1870258 Compare July 25, 2023 13:16

annakhm reviewed Jul 25, 2023

View reviewed changes

ksamoray force-pushed the compute_managers branch from 1870258 to 67c9f66 Compare July 26, 2023 14:09

annakhm reviewed Jul 27, 2023

View reviewed changes

ksamoray force-pushed the compute_managers branch 2 times, most recently from 3c7085e to 7b5f44c Compare July 30, 2023 11:40

ksamoray added 2 commits July 30, 2023 14:42

Implement edge cluster resource

c39be23

Signed-off-by: Kobi Samoray <[email protected]>

Implement compute manager resource

dd53510

Signed-off-by: Kobi Samoray <[email protected]>

ksamoray force-pushed the compute_managers branch from 7b5f44c to dd53510 Compare July 30, 2023 11:43

annakhm reviewed Aug 1, 2023

View reviewed changes

annakhm approved these changes Aug 1, 2023

View reviewed changes

ksamoray merged commit d78635f into vmware:master Aug 3, 2023
2 checks passed

ksamoray deleted the compute_managers branch August 3, 2023 05:19

Implement compute manager resource #920

Implement compute manager resource #920

Conversation

ksamoray commented Jul 12, 2023

vmwclabot commented Jul 12, 2023

ksamoray commented Jul 13, 2023

ksamoray commented Jul 14, 2023

ksamoray commented Jul 17, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ksamoray commented Jul 18, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ksamoray commented Jul 30, 2023

Choose a reason for hiding this comment

ksamoray Aug 2, 2023 • edited Loading

Choose a reason for hiding this comment

annakhm left a comment

Choose a reason for hiding this comment

ksamoray Aug 2, 2023 •

edited

Loading